fix(vendor)!: direct extraction for registry sources #15514

weihanglo · 2025-05-10T05:52:52Z

What does this PR try to resolve?

PathSource::list_files has some heurstic rules for listing files. Those rules are mainly designed for cargo package.

Previously, cargo-vendor relies on those rules to understand what files to vendor. However, it shouldn't use those rules because:

Package extracted from a .crate tarball isn't Git-controlled, some rules may apply differently.
The extracted package already went through PathSource::list_files during packaging. It should be clean enough.
Should keep crate sources from registry sources in a pristine state, which is exactly what vendoring is meant for.

Instead, we switch to direct extraction into the vendor directory
to ensure source code is the same as in the .crate tarball.

How should we test and review this PR?

There is already a test vendor::package_exclude for the fix of #9054,
though now I think it is not a good fix. The test change shows the correct behavior change.

I am not sure if we want more tests.

Also, there are some caveats with this fix:

The overwrite protection in unpack_package assumes the unpack
directory is always <pkg>-<version>.
We don't want to remove this,
but for cargo-vendor supports vendoring without version suffix.
For that case, we need a temporary staging area,
and move the unpacked source then.
The heurstic in PathSource::list_files did something "good" in
general cases, like excluding hidden directories. That means
common directories like .github or .config won't be vendored.
After this, those get included. This is another round of churns.
We might want to get other cargo-vendor changes along with this
in one single release.

Additional information

This also opens a door for

Since we are changing vendored source (again), we might want to remove the .rej/.orig special rules in cargo-vendor, as well as look into the new source-dedup vendor dir layout.

Summary Notes

benchmark-result by weihanglo

Generated by triagebot, see help for how to add more

rustbot · 2025-05-10T05:52:56Z

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

epage · 2025-05-11T00:54:26Z

Except this doesn't guarantee things are pristine until we vendor the contents of the .crate. I worry this might cause us to vendor things like target/ directories.

weihanglo · 2025-05-11T01:27:35Z

Except this doesn't guarantee things are pristine until we vendor the contents of the .crate. I worry this might cause us to vendor things like target/ directories.

The chance of this happening is low because cargo-vendor holds a MutateExclusive cache lock during the entire call. Also basically everything is the same except using walkdir instead of PathSource::list_files. We can add a filter to walkdir to exclude target/ directory, if that helps.

This patch aims to be as simple as possible. The “direct extraction” is the correct fix but require more architectural changes.

epage · 2025-05-11T01:42:05Z

The chance of this happening is low because cargo-vendor holds a MutateExclusive cache lock during the entire call. Also basically everything is the same except using walkdir instead of PathSource::list_files. We can add a filter to walkdir to exclude target/ directory, if that helps.

And that we delete the extracted .crate so we get a fresh extraction.

So looking over this again, this does not fix #15090. This PR only changes which files we list, it doesn't change how we copy them which is what is causing the problem in #15090.

The “direct extraction” is the correct fix but require more architectural changes.

Is it all that big of an architectural change? Can we get an instance of the registry source and and call an extract method directly on it?

weihanglo · 2025-05-11T02:08:05Z

So looking over this again, this does not fix #15090. This PR only changes which files we list, it doesn't change how we copy them which is what is causing the problem in #15090.

How does it not fix #15090? cargo-vendor doesn't change any content of a registry package, at least cp_sources() doesn't. It does skip some files and create a .cargo-checksum.json though.

Is it all that big of an architectural change? Can we get an instance of the registry source and and call an extract method directly on it?

Maybe not really an architectural change, but we'll need to have an abstraction over local/remote/http registry (which is RegistryData/RegistrySource). Ensure .crates be prepared. And decouple unpack_package and get_pkg from the actual download logic. And then we still need to walk the extraced content to compute checksum. Not architectural but also not small.

weihanglo · 2025-05-11T19:29:07Z

Anyway, I got direct extraction working. Doesn't look too terrible than I thought!

weihanglo · 2025-05-11T23:21:55Z

@rustbot note benchmark-result

Also, the benchmark result shows that it is ~60% faster when vendoring cargo and its dependencies. On Apple M1 Pro with 500GiB APFS SSD

Benchmark 1: cargo-15514 vendor
  Time (mean ± σ):     12.331 s ±  0.221 s    [User: 2.705 s, System: 7.211 s]
  Range (min … max):   12.083 s … 12.753 s    10 runs

Benchmark 2: cargo-15514 vendor --versioned-dirs
  Time (mean ± σ):     12.737 s ±  0.242 s    [User: 2.704 s, System: 7.681 s]
  Range (min … max):   12.481 s … 13.240 s    10 runs

Benchmark 3: cargor-master vendor
  Time (mean ± σ):     19.639 s ±  0.475 s    [User: 2.947 s, System: 12.391 s]
  Range (min … max):   19.006 s … 20.481 s    10 runs

Benchmark 4: cargor-master vendor --versioned-dirs
  Time (mean ± σ):     19.574 s ±  0.558 s    [User: 2.948 s, System: 12.422 s]
  Range (min … max):   18.739 s … 20.489 s    10 runs

Summary
  cargofast vendor ran
    1.03 ± 0.03 times faster than cargo-15514 vendor --versioned-dirs
    1.59 ± 0.05 times faster than cargo-master vendor --versioned-dirs
    1.59 ± 0.05 times faster than cargo-master vendor

ehuss

Nice!

I have not gone over this with a fine-toothed comb, but the overall structure looks good to me.

I'm not going to click merge right now in case anyone else wants to look closer, or you had any additional concerns or changes you were thinking of. If there isn't more progress by sometime next week, then I think it would be fine to just merge.

weihanglo · 2025-05-20T20:53:01Z

@ehuss just check in and see if we can merge this. I'd also like to do a follow-up for #13191.

src/cargo/ops/vendor.rs

epage · 2025-05-21T14:31:58Z

This is early enough in the release that I'm going to go ahead and merge this now.

epage · 2025-05-21T14:34:44Z

src/cargo/ops/vendor.rs

+/// Filters files we want to vendor.
+///
+/// `relative` is a path relative to the package root.
+fn vendor_this(relative: &Path) -> bool {


Should we filter out .cargo_vcs_info.json and explicitly bring it back in?

Also, we're closing #11000 without having a test for that case

Got it rearranged.

This commit didn't get into the merge. It said it was disabled.

That was confusing. I approved your updates.

Asked in #t-infra > [Cargo] new commit didn't dismiss auto-merge

It is supposed to be vendored when we do direct extraction.

`PathSource::list_files` has some heurstic rules for listing files. Those rules are mainly designed for `cargo package`. Previously, cargo-vendor relies on those rules to understand what files to vendor. However, it shouldn't use those rules because: * Package extracted from a `.crate` tarball isn't Git-controlled, some rules may apply differently. * The extracted package already went through `PathSource::list_files` during packaging. It should be clean enough. * Should keep crate sources from registry sources in a pristine state, which is exactly what vendoring is meant for. Instead, we switch to direct extraction into the vendor directory to ensure source code is the same as in the `.crate` tarball. There are some caveats: * The overwrite protection in `unpack_package` assumes the unpack directory is always `<pkg>-<version`>. We don't want to remove this, but for cargo-vendor supports vendoring without version suffix. For that case, we need a temporary staging area, and move the unpacked source then. * The heurstic in `PathSource::list_files` did something "good" in general cases, like excluding hidden directories. That means common directorys like `.github` or `.config` won't be vendored. After this, those get included. This is another round of churns. We might want to get other `cargo-vendor` changes along with this in one single release.

To make the behavior change outstanding for fixing rust-lang/rust-lang#11000 this is split into a separate commit.

By doing direct extractions, source code is kept in its original state The old workaround is irrelevant now.

Not sure if it is really needed, though Cargo had better follow what platform support says. > The behavior on Windows is the same on Windows 10 1607 and higher > if `FileRenameInfoEx` is supported by the filesystem; otherwise, > `from` can be anything, but `to` must *not* be a directory. https://doc.rust-lang.org/1.86.0/src/std/fs.rs.html#2430-2435

### What does this PR try to resolve? This is meant to fixes #13191 As git sources and registry sources are considered immutable. I don't think there is any reason excluding those files. There might be a little chance local Git repositories might have those, though that is a rare use case. Alternatively, we could reject all `.rej`/`.orig` files but `Cargo.toml.orig`. ### How should we test and review this PR? Test updates should be sufficient. ### Additional information This is a follow-up of #15514

rustbot assigned ehuss May 10, 2025

rustbot added Command-vendor S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 10, 2025

weihanglo mentioned this pull request May 10, 2025

Normalization of Cargo.toml can break crates with deps. that read Cargo.toml #13191

Closed

weihanglo force-pushed the vendor-pristine branch from c6773a8 to a89f513 Compare May 11, 2025 19:17

rustbot added the A-registries Area: registries label May 11, 2025

weihanglo force-pushed the vendor-pristine branch from a89f513 to b09d3de Compare May 11, 2025 19:24

weihanglo changed the title ~~fix(vendor): dont use PathSource heuristic when copying files~~ fix(vendor)!: direct extraction for registry sources May 11, 2025

ehuss approved these changes May 12, 2025

View reviewed changes

weihanglo mentioned this pull request May 12, 2025

cargo vendor should vendor crates in a pristine state #15090

Closed

epage reviewed May 20, 2025

View reviewed changes

src/cargo/ops/vendor.rs Show resolved Hide resolved

weihanglo force-pushed the vendor-pristine branch from b2fbd65 to 0a5e0fc Compare May 20, 2025 22:14

epage reviewed May 21, 2025

View reviewed changes

src/cargo/ops/vendor.rs Show resolved Hide resolved

weihanglo added 4 commits May 21, 2025 10:07

refactor(registry): extract the general unpack logic

a506ceb

refactor(vendor): remove unnecessary allocations

d760d6e

refactor(vendor): name variables better

8fee50a

refactor(vendor): extract vendor filter to a function

060ab41

weihanglo force-pushed the vendor-pristine branch from 0a5e0fc to c2d3bc4 Compare May 21, 2025 14:11

epage approved these changes May 21, 2025

View reviewed changes

epage enabled auto-merge May 21, 2025 14:32

epage reviewed May 21, 2025

View reviewed changes

epage added this pull request to the merge queue May 21, 2025

weihanglo added 5 commits May 21, 2025 10:49

test(vendor): show .cargo_vsc_info.json not vendored

12c8ba5

It is supposed to be vendored when we do direct extraction.

fix(vendor)!: dont exclude .cargo_vcs_info.json

0eeedae

To make the behavior change outstanding for fixing rust-lang/rust-lang#11000 this is split into a separate commit.

fix(vendor): remove workaround for rust-lang#5956

79d17e7

By doing direct extractions, source code is kept in its original state The old workaround is irrelevant now.

weihanglo force-pushed the vendor-pristine branch from c2d3bc4 to 0290e40 Compare May 21, 2025 14:59

auto-merge was automatically disabled May 21, 2025 15:03
Pull Request is not mergeable

epage approved these changes May 21, 2025

View reviewed changes

Merged via the queue into rust-lang:master with commit ee08992 May 21, 2025

weihanglo deleted the vendor-pristine branch May 21, 2025 15:59

weihanglo mentioned this pull request May 21, 2025

fix(vendor)!: vendor files with .rej/.orig suffix #15569

Merged

fix(vendor)!: direct extraction for registry sources #15514

fix(vendor)!: direct extraction for registry sources #15514

Uh oh!

Conversation

weihanglo commented May 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR try to resolve?

How should we test and review this PR?

Additional information

Summary Notes

Uh oh!

rustbot commented May 10, 2025

Uh oh!

epage commented May 11, 2025

Uh oh!

weihanglo commented May 11, 2025

Uh oh!

epage commented May 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weihanglo commented May 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weihanglo commented May 11, 2025

Uh oh!

weihanglo commented May 11, 2025

Uh oh!

ehuss left a comment

Choose a reason for hiding this comment

Uh oh!

weihanglo commented May 20, 2025

Uh oh!

Uh oh!

Uh oh!

epage commented May 21, 2025

Uh oh!

epage May 21, 2025

Choose a reason for hiding this comment

Uh oh!

weihanglo May 21, 2025

Choose a reason for hiding this comment

Uh oh!

weihanglo May 21, 2025

Choose a reason for hiding this comment

Uh oh!

epage May 21, 2025

Choose a reason for hiding this comment

Uh oh!

weihanglo May 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

weihanglo commented May 10, 2025 •

edited

Loading

epage commented May 11, 2025 •

edited

Loading

weihanglo commented May 11, 2025 •

edited

Loading